GH-1192 Extend /back functionality to per-death and per-teleport #1192

Jakubk15 · 2025-10-04T16:45:56Z

Described in #1133

Resolves #1133

gemini-code-assist

Code Review

This pull request extends the /back command to differentiate between death and teleport locations, which is a great feature enhancement. The implementation is mostly solid, but I've found a critical issue in the configuration migration that could lead to data loss for users. Additionally, there are several opportunities for refactoring to reduce code duplication and improve readability, particularly in BackCommand and BackService. I've also noted a design consideration regarding a shared configuration setting that could be improved.

...eternalcode/core/configuration/migrations/Migration_0010_Move_back_to_dedicated_section.java

gemini-code-assist · 2025-10-04T16:48:23Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackCommand.java

+    public void executeBackDeath(@Sender Player player) {
+        Optional<Pair<BackLocation, BackLocation>> backPair = this.backService.getBackLocationPair(player.getUniqueId());
+
+        if (backPair.isEmpty() || backPair.get().getFirst() == null) {
+            this.noticeService.player(player.getUniqueId(), translation -> translation.back().lastLocationNoExist());
+            return;
+        }
+
+        BackLocation deathLocation = backPair.get().getFirst();
+        this.backService.teleportBack(player, deathLocation.location());
+        this.noticeService.player(player.getUniqueId(), translation -> translation.back().teleportedToLastLocation());
+    }


This method's logic can be simplified by using Optional's functional style methods, making the code more concise and readable. This pattern can also be applied to executeBackTeleport, and both could be extracted into a single helper method to reduce code duplication.

public void executeBackDeath(@Sender Player player) { this.backService.getBackLocationPair(player.getUniqueId()) .map(Pair::getFirst) .filter(java.util.Objects::nonNull) .ifPresentOrElse(deathLocation -> { this.backService.teleportBack(player, deathLocation.location()); this.noticeService.player(player.getUniqueId(), translation -> translation.back().teleportedToLastLocation()); }, () -> this.noticeService.player(player.getUniqueId(), translation -> translation.back().lastLocationNoExist())); }

doesn't look simplified at all

gemini-code-assist · 2025-10-04T16:48:23Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackCommand.java

+    public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) {
+        Optional<Pair<BackLocation, BackLocation>> backPair = this.backService.getBackLocationPair(target.getUniqueId());
+
+        if (backPair.isEmpty() || backPair.get().getFirst() == null) {
+            this.noticeService.viewer(viewer, translation -> translation.back().lastLocationNoExist());
+            return;
+        }
+
+        BackLocation deathLocation = backPair.get().getFirst();
+        this.backService.teleportBack(target, deathLocation.location());
+        this.noticeService.player(target.getUniqueId(), translation -> translation.back().teleportedToLastLocation());
+
+        this.noticeService.create()
+            .viewer(viewer)
+            .notice(translation -> translation.back().teleportedSpecifiedPlayerLastLocation())
+            .placeholder("{PLAYER}", target.getName())
+            .send();
+    }


This method contains logic that is very similar to executeBackTeleportOther and could be refactored to reduce duplication. Additionally, using Optional's functional methods can make the implementation cleaner and more readable.

public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) { this.backService.getBackLocationPair(target.getUniqueId()) .map(Pair::getFirst) .filter(java.util.Objects::nonNull) .ifPresentOrElse(deathLocation -> { this.backService.teleportBack(target, deathLocation.location()); this.noticeService.player(target.getUniqueId(), translation -> translation.back().teleportedToLastLocation()); this.noticeService.create() .viewer(viewer) .notice(translation -> translation.back().teleportedSpecifiedPlayerLastLocation()) .placeholder("{PLAYER}", target.getName()) .send(); }, () -> this.noticeService.viewer(viewer, translation -> translation.back().lastLocationNoExist())); }

gemini-code-assist · 2025-10-04T16:48:23Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackService.java

+    public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) {
+        Pair<BackLocation, BackLocation> existing = backLocationsCache.getIfPresent(playerId);
+        BackLocation newLocation = new BackLocation(location);
+        BackLocation deathLocation;
+        BackLocation normalLocation;
+        if (existing == null) {
+            deathLocation = isFromDeath ? newLocation : null;
+            normalLocation = isFromDeath ? null : newLocation;
+        }
+        else {
+            deathLocation = isFromDeath ? newLocation : existing.getFirst();
+            normalLocation = isFromDeath ? existing.getSecond() : newLocation;
+        }
+        backLocationsCache.put(playerId, Pair.of(deathLocation, normalLocation));
+    }


The logic in this method is a bit complex and can be hard to follow. It can be refactored to be more straightforward and readable, which improves maintainability.

public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) { Pair<BackLocation, BackLocation> pair = Optional.ofNullable(this.backLocationsCache.getIfPresent(playerId)) .orElse(Pair.of(null, null)); BackLocation newLocation = new BackLocation(location); if (isFromDeath) { this.backLocationsCache.put(playerId, Pair.of(newLocation, pair.getSecond())); } else { this.backLocationsCache.put(playerId, Pair.of(pair.getFirst(), newLocation)); } }

gemini-code-assist · 2025-10-04T16:48:23Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackService.java

+                player.getUniqueId(),
+                PositionAdapter.convert(player.getLocation()),
+                PositionAdapter.convert(location),
+                settings.tpaTimer()


The teleport delay for the /back command uses settings.tpaTimer(), which is a setting from the teleport request (/tpa) feature. It would be better to introduce a dedicated configuration setting for the /back command's teleport delay, for example backTimer. This would give server administrators more granular control over teleport delays for different features.

…ion/migrations/Migration_0010_Move_back_to_dedicated_section.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

imDMK · 2025-10-04T18:41:52Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackCommand.java

+import panda.std.Pair;
+
+@Command(name = "back")
+public class BackCommand {


I don't like this class. I would suggest something like this;

Suggested change

public class BackCommand {

import com.eternalcode.annotations.scan.command.DescriptionDocs;

import com.eternalcode.core.feature.back.BackService;

import com.eternalcode.core.feature.back.BackService.BackLocation;

import com.eternalcode.core.injector.annotations.Inject;

import com.eternalcode.core.notice.NoticeService;

import com.eternalcode.core.viewer.Viewer;

import dev.rollczi.litecommands.annotations.argument.Arg;

import dev.rollczi.litecommands.annotations.command.Command;

import dev.rollczi.litecommands.annotations.context.Sender;

import dev.rollczi.litecommands.annotations.execute.Execute;

import dev.rollczi.litecommands.annotations.permission.Permission;

import org.bukkit.entity.Player;

import panda.std.Pair;

import java.util.Optional;

import java.util.UUID;

import java.util.function.Function;

@Command(name = "back")

public class BackCommand {

private static final String PLAYER_PLACEHOLDER = "{PLAYER}";

private final BackService backService;

private final NoticeService noticeService;

@Inject

public BackCommand(BackService backService, NoticeService noticeService) {

this.backService = backService;

this.noticeService = noticeService;

}

@Execute(name = "death")

@Permission("eternalcore.back.death")

@DescriptionDocs(description = "Teleport to your last death location")

public void executeBackDeath(@Sender Player player) {

teleportSelf(player, this::pickDeath);

}

@Execute(name = "teleport")

@Permission("eternalcore.back.teleport")

@DescriptionDocs(description = "Teleport to your last teleport location")

public void executeBackTeleport(@Sender Player player) {

teleportSelf(player, this::pickTeleport);

}

@Execute(name = "death")

@Permission("eternalcore.back.death.other")

@DescriptionDocs(description = "Teleport specified player to their last death location", arguments = "<player>")

public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) {

teleportOther(viewer, target, this::pickDeath);

}

@Execute(name = "teleport")

@Permission("eternalcore.back.teleport.other")

@DescriptionDocs(description = "Teleport specified player to their last teleport location", arguments = "<player>")

public void executeBackTeleportOther(@Sender Viewer viewer, @Arg Player target) {

teleportOther(viewer, target, this::pickTeleport);

}

private void teleportSelf(Player player, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {

resolveBack(player.getUniqueId(), selector).ifPresentOrElse(

loc -> {

this.backService.teleportBack(player, loc.location());

this.noticeService.player(player.getUniqueId(),

t -> t.back().teleportedToLastLocation());

},

() -> this.noticeService.player(player.getUniqueId(),

t -> t.back().lastLocationNoExist())

);

}

private void teleportOther(Viewer viewer, Player target, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {

resolveBack(target.getUniqueId(), selector).ifPresentOrElse(

loc -> {

this.backService.teleportBack(target, loc.location());

this.noticeService.player(target.getUniqueId(),

t -> t.back().teleportedToLastLocation());

this.noticeService.create()

.viewer(viewer)

.notice(t -> t.back().teleportedSpecifiedPlayerLastLocation())

.placeholder(PLAYER_PLACEHOLDER, target.getName())

.send();

},

() -> this.noticeService.viewer(viewer, t -> t.back().lastLocationNoExist())

);

}

private Optional<BackLocation> resolveBack(UUID playerId, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {

return this.backService.getBackLocationPair(playerId)

.map(selector)

.filter(loc -> loc != null);

}

private BackLocation pickDeath(Pair<BackLocation, BackLocation> pair) {

return pair.getFirst();

}

private BackLocation pickTeleport(Pair<BackLocation, BackLocation> pair) {

return pair.getSecond();

}

}

imDMK · 2025-10-05T19:02:40Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackService.java

+    public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) {
+        Pair<BackLocation, BackLocation> existing = backLocationsCache.getIfPresent(playerId);
+        BackLocation newLocation = new BackLocation(location);
+        BackLocation deathLocation;
+        BackLocation normalLocation;
+        if (existing == null) {
+            deathLocation = isFromDeath ? newLocation : null;
+            normalLocation = isFromDeath ? null : newLocation;
+        }
+        else {
+            deathLocation = isFromDeath ? newLocation : existing.getFirst();
+            normalLocation = isFromDeath ? existing.getSecond() : newLocation;
+        }
+        backLocationsCache.put(playerId, Pair.of(deathLocation, normalLocation));
+    }


Suggested change

public void setBackLocation(UUID playerId, Location location, boolean isFromDeath) {

Pair<BackLocation, BackLocation> existing = backLocationsCache.getIfPresent(playerId);

BackLocation newLocation = new BackLocation(location);

BackLocation deathLocation;

BackLocation normalLocation;

if (existing == null) {

deathLocation = isFromDeath ? newLocation : null;

normalLocation = isFromDeath ? null : newLocation;

}

else {

deathLocation = isFromDeath ? newLocation : existing.getFirst();

normalLocation = isFromDeath ? existing.getSecond() : newLocation;

}

backLocationsCache.put(playerId, Pair.of(deathLocation, normalLocation));

}

public void setBackLocation(UUID playerId, Location location, boolean fromDeath) {

Pair<BackLocation, BackLocation> cached = backLocationsCache.getIfPresent(playerId);

BackLocation newLoc = new BackLocation(location);

BackLocation deathLoc = (cached != null) ? cached.getFirst() : null;

BackLocation normalLoc = (cached != null) ? cached.getSecond() : null;

if (fromDeath) {

deathLoc = newLoc;

} else {

normalLoc = newLoc;

}

backLocationsCache.put(playerId, Pair.of(deathLoc, normalLoc));

}

imDMK · 2025-10-05T19:02:59Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/back/BackService.java

+    }
+
+    public void teleportBack(Player player, Location location) {
+        if (player.hasPermission("eternalcore.teleport.bypass")) {


Move to private static variable

vLuckyyy and others added 2 commits September 13, 2025 22:19

WIP permission for back on death.

ac0a158

Improve back logic

32bce4b

Jakubk15 requested review from Rollczi, vLuckyyy, igoyek, P1otrulla, CitralFlo and imDMK October 4, 2025 16:45

Jakubk15 self-assigned this Oct 4, 2025

Jakubk15 requested a review from a team as a code owner October 4, 2025 16:45

Jakubk15 added the 🔧 enhancement Enhances existing function label Oct 4, 2025

Merge branch 'master' into back-on-deatch-permission-wip

1b130ad

Jakubk15 changed the title ~~Extend /back functionality to per-death and per-teleport~~ GH-1192 Extend /back functionality to per-death and per-teleport Oct 4, 2025

gemini-code-assist bot reviewed Oct 4, 2025

View reviewed changes

Update eternalcore-core/src/main/java/com/eternalcode/core/configurat…

c733197

…ion/migrations/Migration_0010_Move_back_to_dedicated_section.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

imDMK requested changes Oct 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

GH-1192 Extend /back functionality to per-death and per-teleport #1192

GH-1192 Extend /back functionality to per-death and per-teleport #1192

Jakubk15 commented Oct 4, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

gemini-code-assist bot Oct 4, 2025

Uh oh!

Jakubk15 Oct 5, 2025

Uh oh!

gemini-code-assist bot Oct 4, 2025

Uh oh!

gemini-code-assist bot Oct 4, 2025

Uh oh!

gemini-code-assist bot Oct 4, 2025

Uh oh!

imDMK Oct 4, 2025

Uh oh!

imDMK Oct 5, 2025

Uh oh!

imDMK Oct 5, 2025

Uh oh!

Uh oh!

-public class BackCommand {
+import com.eternalcode.annotations.scan.command.DescriptionDocs;
+import com.eternalcode.core.feature.back.BackService;
+import com.eternalcode.core.feature.back.BackService.BackLocation;
+import com.eternalcode.core.injector.annotations.Inject;
+import com.eternalcode.core.notice.NoticeService;
+import com.eternalcode.core.viewer.Viewer;
+import dev.rollczi.litecommands.annotations.argument.Arg;
+import dev.rollczi.litecommands.annotations.command.Command;
+import dev.rollczi.litecommands.annotations.context.Sender;
+import dev.rollczi.litecommands.annotations.execute.Execute;
+import dev.rollczi.litecommands.annotations.permission.Permission;
+import org.bukkit.entity.Player;
+import panda.std.Pair;
+import java.util.Optional;
+import java.util.UUID;
+import java.util.function.Function;
+@Command(name = "back")
+public class BackCommand {
+    private static final String PLAYER_PLACEHOLDER = "{PLAYER}";
+    private final BackService backService;
+    private final NoticeService noticeService;
+    @Inject
+    public BackCommand(BackService backService, NoticeService noticeService) {
+        this.backService = backService;
+        this.noticeService = noticeService;
+    }
+    @Execute(name = "death")
+    @Permission("eternalcore.back.death")
+    @DescriptionDocs(description = "Teleport to your last death location")
+    public void executeBackDeath(@Sender Player player) {
+        teleportSelf(player, this::pickDeath);
+    }
+    @Execute(name = "teleport")
+    @Permission("eternalcore.back.teleport")
+    @DescriptionDocs(description = "Teleport to your last teleport location")
+    public void executeBackTeleport(@Sender Player player) {
+        teleportSelf(player, this::pickTeleport);
+    }
+    @Execute(name = "death")
+    @Permission("eternalcore.back.death.other")
+    @DescriptionDocs(description = "Teleport specified player to their last death location", arguments = "<player>")
+    public void executeBackDeathOther(@Sender Viewer viewer, @Arg Player target) {
+        teleportOther(viewer, target, this::pickDeath);
+    }
+    @Execute(name = "teleport")
+    @Permission("eternalcore.back.teleport.other")
+    @DescriptionDocs(description = "Teleport specified player to their last teleport location", arguments = "<player>")
+    public void executeBackTeleportOther(@Sender Viewer viewer, @Arg Player target) {
+        teleportOther(viewer, target, this::pickTeleport);
+    }
+    private void teleportSelf(Player player, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
+        resolveBack(player.getUniqueId(), selector).ifPresentOrElse(
+            loc -> {
+                this.backService.teleportBack(player, loc.location());
+                this.noticeService.player(player.getUniqueId(),
+                    t -> t.back().teleportedToLastLocation());
+            },
+            () -> this.noticeService.player(player.getUniqueId(),
+                    t -> t.back().lastLocationNoExist())
+        );
+    }
+    private void teleportOther(Viewer viewer, Player target, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
+        resolveBack(target.getUniqueId(), selector).ifPresentOrElse(
+            loc -> {
+                this.backService.teleportBack(target, loc.location());
+                this.noticeService.player(target.getUniqueId(),
+                    t -> t.back().teleportedToLastLocation());
+                this.noticeService.create()
+                    .viewer(viewer)
+                    .notice(t -> t.back().teleportedSpecifiedPlayerLastLocation())
+                    .placeholder(PLAYER_PLACEHOLDER, target.getName())
+                    .send();
+            },
+            () -> this.noticeService.viewer(viewer, t -> t.back().lastLocationNoExist())
+        );
+    }
+    private Optional<BackLocation> resolveBack(UUID playerId, Function<Pair<BackLocation, BackLocation>, BackLocation> selector) {
+        return this.backService.getBackLocationPair(playerId)
+            .map(selector)
+            .filter(loc -> loc != null);
+    }
+    private BackLocation pickDeath(Pair<BackLocation, BackLocation> pair) {
+        return pair.getFirst();
+    }
+    private BackLocation pickTeleport(Pair<BackLocation, BackLocation> pair) {
+        return pair.getSecond();
+    }
+}

Uh oh!

GH-1192 Extend /back functionality to per-death and per-teleport #1192

Are you sure you want to change the base?

GH-1192 Extend /back functionality to per-death and per-teleport #1192

Conversation

Jakubk15 commented Oct 4, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Jakubk15 Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

imDMK Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

imDMK Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

imDMK Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!